"files.defaultLanguage": "powershell",
    "[powershell]": {
        "editor.rulers": [ 120 ]
    },
    "[markdown]": {
        "editor.rulers": [ 120 ]
    },

                    $samAccountNameSplit = $member -split '\\'
                    $domain = $samAccountNameSplit[0]
                    $accountName = $samAccountNameSplit[1]
                    $ntAccount = [System.Security.Principal.NTAccount]::new($domain, $accountName)

    "files.defaultLanguage": "powershell",
    "[powershell]": {
        "editor.rulers": [ 120 ]
    },
    "[markdown]": {
        "editor.rulers": [ 120 ]
    },

Please remove these. We can consider setting these, but in their own PR.

Please format all the hash tables you have added so that the attributes and values align. Format Document in VSCode will do it.

Please rename to $setADCommonGroupMemberParms and below.

Please rename this to $resolveMembersSecurityIdentifierParms

... for easier one-way trust AD group membership ...

                    $translateSamAccountNameMessage = $script:localizedData.TranslatingMembershipAttribute -f
                        $MembershipAttribute, $member, $property
                    Write-Verbose -Message "  $($translateSamAccountNameMessage)" -Verbose:$verbose

You can split PowerShell lines naturally at the -f parameter so this and otherWrite-Verbose messages can be written as:

Write-Verbose -Message $script:localizedData.ResolvingMembershipAttributeValues -f
    $property, $MembershipAttribute -Verbose:$verbose

Can you rename this to $getADObjectParms

Can you change this to foreach ($member in $Members)

I don't like try/catch blocks covering large amounts of code. This should only cover calls that are likely to fail and deal with them explicitly. Can you please re-think this?

                    $samAccountNameSplit = $member -split '\\'
                    $domain = $samAccountNameSplit[0]
                    $accountName = $samAccountNameSplit[1]
                    $ntAccount = [System.Security.Principal.NTAccount]::new($domain, $accountName)

You don't need to split the qualified account. This should work:

$ntAccount = [System.Security.Principal.NTAccount]::new($member)

Please rename to $getADObjectParms

The only purpose they serve is code coverage. Are we foregoing that?

Yes that's fine. We don't have tests for the other wrapper functions.

No need for WarningAction

You can move this Assert-Module mock to a BeforeAll block in the Describe block and then there is no need to specify it in each Context block.

Please add Mock and Assert-MockCalled for Resolve-SecurityIdentifier to all these tests.

Please add -Exactly -Times 1 to all these Assert-MockCalled

Can we remove one indented space.

No need for WarningAction

Can we remove one indented space.

No need for this test as you are not handling this exception in the function.

Please change this to Should throw the correct exception and check the exception on the Should.

Please rename to $resolveMembersSecurityIdentifierParms

No need for this Write-Debug mock.

Please add Assert-MockCalled for each command mocked here and below with ParameterFilterif applicable and -Exactly -Times x.

No need for this Write-Debug mock.

I use this mock to increment the memberIndex so that I can pass an array of members into Resolve-MembersSecurityIdentifier and correctly maintain the index through the looping within.

This line is not longer need.

If I comment out the line the tests still pass, so it is not achieving anything.

As there are some many differing functions to test in this module, can we not share test variables between the tests, but duplicate and move the variables into the Describe block for each test that uses them.

No need for `return and please format as:

        Mock -CommandName Resolve-MembersSecurityIdentifier `
            -MockWith { $membershipSID['member'] }

Please add -Exactly -Times 1

Please add -Exactly -Times $setADCommonGroupMemberParms.Members.Count

Please format as:

                Assert-MockCalled -CommandName Set-ADGroup `
                    -ParameterFilter { $Add -ne $null } `
                    -Exactly -Times 1

Please add -Exactly -Times 1

Please add -Exactly -Times $setADCommonGroupMemberParms.Members.Count

Please format as:

                Assert-MockCalled -CommandName Set-ADGroup `
                    -ParameterFilter { $Remove -ne $null } `
                    -Exactly -Times 1

Please change to When 'Set-ADGroup' throw an exception

Please change to throw 'Error'

Please add:

                $errorMessage = $script:localizedData.FailedToSetADGroupMembership -f
                    $groupMembersParms.Parameters.Identity

Please change to Should throw the correct exception

Please change to Should -Throw $errorMessage

There is no point in having a count increment each time the function is called and then checking the function is called that number of times, you are not achieving anything! This should only be called when the MembershipAttribute is SamAccountNameand the number of times will be the number of members that are domain qualified.

Same for this one. It is not achieving anything. The mock data needs to structured so you can check the correct count of this Mock.

Specify the full $mockADGroupMembersAsADObjects.ObjectGUID array, and remove the $script:memberIndex.

Change this to It 'Calls the correct mocks' {

                Assert-MockCalled -CommandName Assert-Module -ParameterFilter {
                    $ModuleName -eq 'ActiveDirectory'
                } -Exactly -Times 1

To keep the correct indentation, please format here and below as:

                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1

Change to -Exactly -Times $resolveMembersSecurityIdentifierParms.count

Specify the full $mockADGroupMembersAsADObjects.ObjectGUID array, and remove the $script:memberIndex.

Change this to It 'Calls the correct mocks' {

Change to -Exactly -Times $resolveMembersSecurityIdentifierParms.count

Can you have a rethink about how these tests are structured? Trying to use a foreach loop makes the test logic complicated, and the Assert-Mocks -Times parameter is contrived.

Here is a refactored/reformatted Set-ADCommonGroupMember test:

Describe 'ActiveDirectoryDsc.Common\Set-ADCommonGroupMember' {
        BeforeAll {
            $mockADGroupMembersAsADObjects = @(
                [PSCustomObject] @{
                    DistinguishedName = 'CN=User 1,CN=Users,DC=contoso,DC=com'
                    ObjectGUID        = 'a97cc867-0c9e-4928-8387-0dba0c883b8e'
                    SamAccountName    = 'USER1'
                    ObjectSID         = 'S-1-5-21-1131554080-2861379300-292325817-1106'
                    ObjectClass       = 'user'
                }
                [PSCustomObject] @{
                    DistinguishedName = 'CN=Group 1,CN=Users,DC=contoso,DC=com'
                    ObjectGUID        = 'e2328767-2673-40b2-b3b7-ce9e6511df06'
                    SamAccountName    = 'GROUP1'
                    ObjectSID         = 'S-1-5-21-1131554080-2861379300-292325817-1206'
                    ObjectClass       = 'group'
                }
                [PSCustomObject] @{
                    DistinguishedName = 'CN=Computer 1,CN=Users,DC=contoso,DC=com'
                    ObjectGUID        = '42f9d607-0934-4afc-bb91-bdf93e07cbfc'
                    SamAccountName    = 'COMPUTER1'
                    ObjectSID         = 'S-1-5-21-1131554080-2861379300-292325817-6606'
                    ObjectClass       = 'computer'
                }
                # This entry is used to represent a group member from a one-way trusted domain
                [PSCustomObject] @{
                    DistinguishedName = 'CN=S-1-5-21-8562719340-2451078396-046517832-2106,CN=ForeignSecurityPrincipals,DC=contoso,DC=com'
                    ObjectGUID        = '6df78e9e-c795-4e67-a626-e17f1b4a0d8b'
                    SamAccountName    = 'ADATUM\USER1'
                    ObjectSID         = 'S-1-5-21-8562719340-2451078396-046517832-2106'
                    ObjectClass       = 'foreignSecurityPrincipal'
                }
            )

            $setADCommonGroupMemberParms = @{
                Members             = $mockADGroupMembersAsADObjects.DistinguishedName
                MembershipAttribute = 'DistinguishedName'
                Parameters          = @{
                    Identity = 'CN=TestGroup,OU=Fake,DC=contoso,DC=com'
                }
            }

            $membershipSID = @{
                member = $mockADGroupMembersAsADObjects.ObjectSID | ForEach-Object -Process { "<SID=$($_)>" }
            }

            Mock -CommandName Assert-Module
            Mock -CommandName Resolve-MembersSecurityIdentifier -MockWith { $membershipSID['member'] }
            Mock -CommandName Set-ADGroup
        }

        Context "When the 'Action' parameter is specified as 'Add'" {
            BeforeAll {
                $setADCommonGroupMemberAddParms = $setADCommonGroupMemberParms.Clone()
                $setADCommonGroupMemberAddParms['Action'] = 'Add'
            }

            It 'Should not throw' {
                { Set-ADCommonGroupMember @setADCommonGroupMemberAddParms } | Should -Not -Throw
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-MembersSecurityIdentifier `
                    -Exactly -Times $setADCommonGroupMemberAddParms.Members.Count
                Assert-MockCalled -CommandName Set-ADGroup `
                    -ParameterFilter { `
                        $Add -ne $null -and `
                        $Identity -eq $setADCommonGroupMemberAddParms.Parameters.Identity } `
                    -Exactly -Times 1
            }
        }

        Context "When 'Action' parameter is specified as 'Remove'" {
            BeforeAll {
                $setADCommonGroupMemberRemoveParms = $setADCommonGroupMemberParms.Clone()
                $setADCommonGroupMemberRemoveParms['Action'] = 'Remove'
            }

            It 'Should not throw' {
                { Set-ADCommonGroupMember @setADCommonGroupMemberRemoveParms } | Should -Not -Throw
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-MembersSecurityIdentifier `
                    -Exactly -Times $setADCommonGroupMemberRemoveParms.Members.Count
                Assert-MockCalled -CommandName Set-ADGroup `
                    -ParameterFilter { `
                        $Remove -ne $null -and `
                        $Identity -eq $setADCommonGroupMemberRemoveParms.Parameters.Identity } `
                    -Exactly -Times 1
            }
        }

        Context "When 'Set-ADGroup' throws an exception" {
            BeforeAll {
                Mock -CommandName Set-ADGroup -MockWith { throw 'Error' }

                $errorMessage = $script:localizedData.FailedToSetADGroupMembership -f
                    $setADCommonGroupMemberParms.Parameters.Identity
            }

            It "Should throw the correct exception" {
                { Set-ADCommonGroupMember @setADCommonGroupMemberParms } |
                    Should -Throw $errorMessage
            }
        }
    }

Here is a refactored/reformatted Resolve-MembersSecurityIdentifier test:

Describe 'ActiveDirectoryDsc.Common\Resolve-MembersSecurityIdentifier' {
        $mockADGroupMembersAsADObjects = @(
            [PSCustomObject] @{
                DistinguishedName = 'CN=User 1,CN=Users,DC=contoso,DC=com'
                ObjectGUID        = 'a97cc867-0c9e-4928-8387-0dba0c883b8e'
                SamAccountName    = 'USER1'
                ObjectSID         = 'S-1-5-21-1131554080-2861379300-292325817-1106'
                ObjectClass       = 'user'
            }
            [PSCustomObject] @{
                DistinguishedName = 'CN=Group 1,CN=Users,DC=contoso,DC=com'
                ObjectGUID        = 'e2328767-2673-40b2-b3b7-ce9e6511df06'
                SamAccountName    = 'GROUP1'
                ObjectSID         = 'S-1-5-21-1131554080-2861379300-292325817-1206'
                ObjectClass       = 'group'
            }
            [PSCustomObject] @{
                DistinguishedName = 'CN=Computer 1,CN=Users,DC=contoso,DC=com'
                ObjectGUID        = '42f9d607-0934-4afc-bb91-bdf93e07cbfc'
                SamAccountName    = 'COMPUTER1'
                ObjectSID         = 'S-1-5-21-1131554080-2861379300-292325817-6606'
                ObjectClass       = 'computer'
            }
            # This entry is used to represent a group member from a one-way trusted domain
            [PSCustomObject] @{
                DistinguishedName = 'CN=S-1-5-21-8562719340-2451078396-046517832-2106,CN=ForeignSecurityPrincipals,DC=contoso,DC=com'
                ObjectGUID        = '6df78e9e-c795-4e67-a626-e17f1b4a0d8b'
                SamAccountName    = 'ADATUM\USER1'
                ObjectSID         = 'S-1-5-21-8562719340-2451078396-046517832-2106'
                ObjectClass       = 'foreignSecurityPrincipal'
            }
        )

        BeforeAll {
            $script:memberIndex = 0

            Mock -CommandName Assert-Module

            Mock -CommandName Resolve-SecurityIdentifier -MockWith {
                $memberADObjectSID = $mockADGroupMembersAsADObjects[($script:memberIndex)].ObjectSID
                $script:memberIndex++
                return $memberADObjectSID
            }

            Mock -CommandName Get-ADObject -MockWith {
                $memberADObject = $mockADGroupMembersAsADObjects[$script:memberIndex]
                $script:memberIndex++
                return $memberADObject
            }
        }

        Context "When 'Server' is passed as part of the 'Parameters' parameter" {
            BeforeAll {
                $testServer = 'TESTDC'
                $membershipAttribute = 'ObjectGUID'

                $script:memberIndex = 0

                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects.$membershipAttribute
                    MembershipAttribute = $membershipAttribute
                    Parameters          = @{
                        Server = $testServer
                    }
                }
            }

            It 'Should not throw' {
                { Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms } | Should -Not -Throw
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-SecurityIdentifier `
                    -Exactly -Times 0
                Assert-MockCalled -CommandName Get-ADObject `
                    -ParameterFilter { $Server -eq $testServer } `
                    -Exactly -Times $mockADGroupMembersAsADObjects.Count
            }
        }

        Context "When 'Credential' is passed as part of the 'Parameters' parameter" {
            BeforeAll {
                $testCredentials = New-Object -TypeName 'System.Management.Automation.PSCredential' -ArgumentList @(
                    'DummyUser',
                    (ConvertTo-SecureString -String 'DummyPassword' -AsPlainText -Force)
                )
                $membershipAttribute = 'ObjectGUID'

                $script:memberIndex = 0

                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects.$membershipAttribute
                    MembershipAttribute = $membershipAttribute
                    Parameters          = @{
                        Credential = $testCredentials
                    }
                }
            }

            It 'Should not throw' {
                { Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms } | Should -Not -Throw
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-SecurityIdentifier `
                    -Exactly -Times 0
                Assert-MockCalled -CommandName Get-ADObject `
                    -ParameterFilter { $Credential -eq $testCredentials } `
                    -Exactly -Times $mockADGroupMembersAsADObjects.Count
            }
        }

        Context "When 'Get-ADObject' returns no value" {
            BeforeAll {
                $membershipAttribute = 'ObjectGUID'

                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects[0].$membershipAttribute
                    MembershipAttribute = $membershipAttribute
                }

                $errorMessage = ($script:localizedData.UnableToResolveMembershipAttribute -f
                    'ObjectSID', $membershipAttribute, $mockADGroupMembersAsADObjects[0].$membershipAttribute)

                Mock -CommandName Resolve-SecurityIdentifier
                Mock -CommandName Get-ADObject
            }

            It 'Should throw the correct exception' {
                { Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms } |
                    Should -Throw $errorMessage
            }
        }

        Context "When MembershipAttribute 'SamAccountName' is specified" {
            BeforeAll {
                $membershipAttribute = 'SamAccountName'

                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects.$membershipAttribute
                    MembershipAttribute = $membershipAttribute
                }

                $resolveSecurityIdentifierCount = @($mockADGroupMembersAsADObjects |
                    Where-Object -Property $membershipAttribute -Match '\\').Count

                $getADObjectCount = @($mockADGroupMembersAsADObjects |
                    Where-Object -Property $membershipAttribute -NotMatch '\\').Count

                $script:memberIndex = 0
            }

            It 'Should return the correct result' {
                $result = Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms

                for ($i = 0; $i -lt $result.Count; $i++)
                {
                    $result[$i] | Should -Be $mockADGroupMembersAsADObjects[$i].ObjectSID
                }
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-SecurityIdentifier `
                    -Exactly -Times $resolveSecurityIdentifierCount
                Assert-MockCalled -CommandName Get-ADObject `
                    -Exactly -Times $getADObjectCount
            }
        }

        Context "When MembershipAttribute 'DistinguishedName' is specified" {
            BeforeAll {
                $membershipAttribute = 'DistinguishedName'

                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects.$membershipAttribute
                    MembershipAttribute = $membershipAttribute
                }

                $getADObjectCount = @($mockADGroupMembersAsADObjects |
                    Where-Object -Property $membershipAttribute -NotMatch 'CN=ForeignSecurityPrincipals').Count

                $script:memberIndex = 0
            }

            It 'Should return the correct result' {
                $result = Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms

                for ($i = 0; $i -lt $result.Count; $i++)
                {
                    $result[$i] | Should -Be $mockADGroupMembersAsADObjects[$i].ObjectSID
                }
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-SecurityIdentifier `
                    -Exactly -Times 0
                Assert-MockCalled -CommandName Get-ADObject `
                    -Exactly -Times $getADObjectCount
            }
        }

        Context "When MembershipAttribute 'ObjectGUID' is specified" {
            BeforeAll {
                $membershipAttribute = 'ObjectGUID'

                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects.$membershipAttribute
                    MembershipAttribute = $membershipAttribute
                }

                $script:memberIndex = 0
            }

            It 'Should Return the correct result' {
                $result = Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms

                for ($i = 0; $i -lt $result.Count; $i++)
                {
                    $result[$i] | Should -Be $mockADGroupMembersAsADObjects[$i].ObjectSID
                }
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-SecurityIdentifier `
                    -Exactly -Times 0
                Assert-MockCalled -CommandName Get-ADObject `
                    -Exactly -Times $mockADGroupMembersAsADObjects.Count
            }
        }

        Context "When MembershipAttribute 'SID' is specified" {
            BeforeAll {
                $resolveMembersSecurityIdentifierParms = @{
                    Members             = $mockADGroupMembersAsADObjects.ObjectSID
                    MembershipAttribute = 'SID'
                }

                $script:memberIndex = 0
            }

            It 'Should return the correct result' {
                $result = Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms

                for ($i = 0; $i -lt $result.Count; $i++)
                {
                    $result[$i] | Should -Be $mockADGroupMembersAsADObjects[$i].ObjectSID
                }
            }

            It 'Should call the expected mocks' {
                Assert-MockCalled -CommandName Assert-Module `
                    -ParameterFilter { $ModuleName -eq 'ActiveDirectory' } `
                    -Exactly -Times 1
                Assert-MockCalled -CommandName Resolve-SecurityIdentifier `
                    -Exactly -Times 0
                Assert-MockCalled -CommandName Get-ADObject `
                    -Exactly -Times 0
            }
        }

        Context "When 'PrepareForMembership' is specified" {
            Context "When the MembershipAttribute specified is not 'SID'" {
                BeforeAll {
                    $membershipAttribute = 'ObjectGUID'

                    $resolveMembersSecurityIdentifierParms = @{
                        Members              = $mockADGroupMembersAsADObjects.$membershipAttribute
                        MembershipAttribute  = $membershipAttribute
                        PrepareForMembership = $true
                    }
                }

                It 'Should return the correct result' {
                    $result = Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms

                    for ($i = 0; $i -lt $result.Count; $i++)
                    {
                        $result[$i] | Should -Be "<SID=$($mockADGroupMembersAsADObjects[$i].ObjectSID)>"
                    }
                }
            }

            Context "When MembershipAttribute specified is 'SID'" {
                BeforeAll {
                    $resolveMembersSecurityIdentifierParms = @{
                        Members              = $mockADGroupMembersAsADObjects.ObjectSID
                        MembershipAttribute  = 'SID'
                        PrepareForMembership = $true
                    }
                }

                It 'Should return the correct result' {
                    $result = Resolve-MembersSecurityIdentifier @resolveMembersSecurityIdentifierParms

                    for ($i = 0; $i -lt $result.Count; $i++)
                    {
                        $result[$i] | Should -Be "<SID=$($mockADGroupMembersAsADObjects[$i].ObjectSID)>"
                    }
                }
            }
        }
    }

Can we format the Assert-MockCalled as:

Assert-MockCalled -CommandName Set-ADCommonGroupMember `
    -ParameterFilter { $Action -eq 'Add' } `
    -Scope It -Exactly -Times 1

here and below to keep the indentation correct.

Can we format this as:

Mock -CommandName Set-ADCommonGroupMember `
    -ParameterFilter { $Action eq - 'Add' }

here and below to keep the indentation correct.